A legibility floor for every colour, and a trigger route that keeps the line where it was - #31
Merged
Merged
Conversation
…a line Two reported defects with one cause. Measured every colour the client can paint against the plane it lands on: only `grey` of the sixteen picker names clears 3:1 on both the dark and light themes, and the freeze bar's accent is 1.27:1 on a focused dark pane -- which is the reported "purple against a blue background". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
…nds on The reported defect is one cell of a grid. The freeze bar took its accent from the theme's index 5 and painted it on the pane: #800080 on a #36363d focused pane is 1.27:1, very nearly the same colour twice. Measured across the whole grid, six of the F2 picker's sixteen names fail 3:1 on the dark theme, nine fail on the light one, and only `grey` clears both -- a palette of fixed hexes cannot serve two themes, so the resolution has to happen where the colour meets its plane rather than where it is chosen. Core.Text.Contrast is that rule: WCAG luminance and ratio, plus Legible(), which moves a foreground the smallest distance that clears the floor and leaves anything already legible byte-identical. Direction is the plane's, not the colour's -- a dark plane lifts and a light plane darkens -- which is what lets one function serve all three themes. Hue survives while there is headroom and then desaturates, because pure #0000ff tops out at 1.88:1 on a dark pane at full blue and a rule that held hue absolutely would leave the commonest unreadable colour in MU* output unreadable. The floor is 3.0:1 and deliberately not 4.5: a game's own de-emphasis is spoken in exactly the colours a 4.5 floor would erase. MarkupFormatter applies it to every foreground it paints, against the span's own background when it has one and the theme's reading plane when it does not. That plane is per *theme*, not per pane -- the extreme of the fourteen a pane can wear, which is the worst case rather than an approximation of one -- so a focus change never re-formats a buffer. F7's `keep text legible` (default on) switches it off to exactly the previous bytes. The client's own hexes become ChromeInk, derived from the theme and held to the same floor. That fixes a second defect nobody had reported: on the Light theme the accent was 1.42:1, the draft pen 1.26:1 and the notice 1.73:1, so its chrome has never been readable -- and no snapshot showed it because every frame in the gallery renders Dark. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
…at means main Four changes, no schema change and no migration. `route` gains an explicit `(none)`: the rule adds no destination and the line follows whatever the other matched rules decided. That is what SpawnTarget=null has always meant -- F2 labelled it `main`, which reads as a destination, so "highlight it and leave it where it was" looked like something the screen could not express. It is the default for a new rule. `main` becomes a real destination: the matching session's own window. It earns a reserved word rather than being spelt as the window's title because one trigger set is shared by every character that lists it, and a title can only name one of them. Gag suppresses the default delivery only. Explicit destinations survive it -- already true of a spawn pane, now true of main -- so `route: main` plus gag keeps the line where before it deleted it. Destinations are deduplicated. They were not, and the session raises one event per entry, so a highlight rule pointed at the same pane as its capture rule delivered every line twice. Also folds in five colour defects the frame audit found after the fact -- the trigger left-rule, the header chip, a world's accent on the rail, the unread badge on a tab, and the command line's ink on Solarized's armed band -- and adds FrameContrastTests, which walks every emitted SGR pair over 24 views x 3 themes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
…-theme Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
RuleRow had its own `?? "main"` beside the route field's. After the field learned that a null target is *no destination* rather than the main window, the list would have gone on calling it `main` -- the two surfaces disagreeing about one rule, which is the shape of the confusion the rename exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
WalkthroughThe change adds WCAG-based theme contrast correction, theme-derived TUI chrome colors, explicit trigger destinations, ChangesTheme contrast and rendering
Trigger routing
Options and snapshots
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/superpowers/specs/2026-08-11-legible-colour-and-trigger-routing-design.md`:
- Line 10: Update the blockquote content in the documentation so the blank
separator line is represented by a `>` marker, preserving continuous quoted
content and satisfying markdownlint MD028.
In `@src/SharpMUTerm.Tui/TriggersScreenRenderer.cs`:
- Line 306: Update the route assignment in the trigger action editor so the
value is trimmed before comparing it with NoRoute. Store null for
whitespace-padded or exact NoRoute values; otherwise store the normalized
trimmed route in entry.Trigger.Actions.SpawnTarget.
In `@src/SharpMUTerm.Tui/WorkspacePalette.cs`:
- Around line 432-436: Update ChromePlane to include HeaderChip(theme) among the
candidate planes passed to Extreme, preserving the existing pane, backdrop, and
surface candidates. Add the corresponding palette assertion for HeaderChip in
every supported theme definition or validation set.
In `@tests/SharpMUTerm.Tui.Tests/FrameContrastTests.cs`:
- Around line 123-156: Update Pairs to locate the next Sgr or Csi match once per
outer iteration, then process the intervening plain-text span with a single
inner loop before handling that escape sequence. Preserve Apply updates, escape
skipping, character filtering, and pair counting while eliminating repeated
Sgr.Match/Csi.Match scans from each character position.
- Around line 98-101: The escape-processing loop in Pairs should avoid calling
Sgr.Match or Csi.Match for ordinary characters. Check frame[i] for '\x1b' before
invoking either regex, while preserving existing handling for escape sequences;
alternatively, parse each escape sequence once and advance past it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4920b7c-138f-4d03-b147-09e3bcdea329
📒 Files selected for processing (33)
CLAUDE.mddocs/superpowers/specs/2026-08-11-legible-colour-and-trigger-routing-design.mdsrc/SharpMUTerm.Core/Automation/Trigger.cssrc/SharpMUTerm.Core/Automation/TriggerEngine.cssrc/SharpMUTerm.Core/Configuration/PreferenceSettings.cssrc/SharpMUTerm.Core/Session/WorldSession.cssrc/SharpMUTerm.Core/Text/Contrast.cssrc/SharpMUTerm.Tui/ChromeInk.cssrc/SharpMUTerm.Tui/MarkupFormatter.cssrc/SharpMUTerm.Tui/OptionsScreenRenderer.cssrc/SharpMUTerm.Tui/PaneDropRenderer.cssrc/SharpMUTerm.Tui/PrefixPanel.cssrc/SharpMUTerm.Tui/Program.cssrc/SharpMUTerm.Tui/RailRenderer.cssrc/SharpMUTerm.Tui/SharpMUTermApp.cssrc/SharpMUTerm.Tui/TabTitles.cssrc/SharpMUTerm.Tui/TriggersScreenRenderer.cssrc/SharpMUTerm.Tui/TriggersScreenView.cssrc/SharpMUTerm.Tui/UnreadBadge.cssrc/SharpMUTerm.Tui/WorkspacePalette.cstests/SharpMUTerm.Core.Tests/ContrastTests.cstests/SharpMUTerm.Core.Tests/TriggerRouteMainTests.cstests/SharpMUTerm.Tui.Tests/FrameContrastTests.cstests/SharpMUTerm.Tui.Tests/LegiblePaletteTests.cstests/SharpMUTerm.Tui.Tests/MarkupFormatterTests.cstests/SharpMUTerm.Tui.Tests/PaneDropRendererTests.cstests/SharpMUTerm.Tui.Tests/ScreenChoiceListTests.cstests/SharpMUTerm.Tui.Tests/ScreenCursorTests.cstests/SharpMUTerm.Tui.Tests/ScreenModelTests.cstests/SharpMUTerm.Tui.Tests/TabActivityIndicatorTests.cstests/SharpMUTerm.Tui.Tests/TabTitlesTests.cstests/SharpMUTerm.Tui.Tests/TriggerKeepItHereTests.cstests/SharpMUTerm.Tui.Tests/TriggersScreenEditingTests.cs
Three of CodeRabbit's five on PR #31. FrameContrastTests.Pairs asked both regexes at every character. Regex.Match searches *forward*, so each plain character re-scanned the same upcoming escape from a later start -- quadratic in the length of every unstyled run, and a frame is mostly padding. It now only asks at an escape; the 72-case suite goes 1.54s -> 1.32s, and the cost stops scaling with frame size. The route trim: the finding was that `v == NoRoute` compares before `v.Trim()`, so a padded "(none)" would be stored as a capture pane by that name. It is not -- ScreenField.WindowName's Set is `value => set(value.Trim())`, so the lambda is handed an already-trimmed value and the `v.Trim()` in it is redundant. Pinned rather than changed, since the two halves of that live in different files and nothing else held them together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/SharpMUTerm.Tui.Tests/FrameContrastTests.cs (1)
131-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDecode the terminal frame before counting contrast pairs.
Pairstreats terminal output as a linear glyph stream. It does not apply cursor movement, erases, or cell overwrites. The test can count text that is not present in the final rendered frame.Replace the custom
SgrandCsiwalker withFrameGrid.Decode. Derive contrast pairs from the final painted cells.Based on learnings: “use the shared
FrameGridhelper for ANSI frame parsing and inspection.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/SharpMUTerm.Tui.Tests/FrameContrastTests.cs` around lines 131 - 174, Replace the custom Sgr/Csi parsing loop used to build contrast pairs with FrameGrid.Decode, then inspect the decoded final painted cells to count only visible glyphs and their foreground/background colors. Remove the bespoke ANSI walker and preserve the existing filtering behavior from Count for spaces, fills, and cells lacking complete color information.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/SharpMUTerm.Tui.Tests/FrameContrastTests.cs`:
- Around line 131-174: Replace the custom Sgr/Csi parsing loop used to build
contrast pairs with FrameGrid.Decode, then inspect the decoded final painted
cells to count only visible glyphs and their foreground/background colors.
Remove the bespoke ANSI walker and preserve the existing filtering behavior from
Count for spaces, fills, and cells lacking complete color information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e90c2008-b832-4423-b907-e4885e77c961
📒 Files selected for processing (3)
docs/superpowers/specs/2026-08-11-legible-colour-and-trigger-routing-design.mdtests/SharpMUTerm.Tui.Tests/FrameContrastTests.cstests/SharpMUTerm.Tui.Tests/TriggersScreenEditingTests.cs
CodeRabbit's outside-diff finding, and it was right about the premise: a frame
is cursor-addressed, so a walker that reads it linearly counts every glyph the
driver *wrote* rather than the ones left on screen.
The literal suggestion (use FrameGrid.Decode) is not implementable -- Decode
keeps glyphs and drops colour, and Backgrounds keeps backgrounds and drops
foregrounds, so neither can answer a contrast question. FrameGrid gains a
colour-aware `Cells` instead, and FrameContrastTests drops its own walker for
it. That also stops this suite being the third copy of a parser FrameGrid's own
remarks warn about ("a suite going quietly green on a frame it has misread").
Measured before changing: on all 72 genuine per-view frames the two walks agree
on the pair set exactly. The linear walk was a superset of the screen, so it
could have raised a false alarm but never missed an offender -- which is the
safe direction for an audit, and is why this is a tidy-up rather than a fix.
Also adds the guard that matters when swapping walkers: a walker handed the
wrong dimensions decodes nothing, and an audit over nothing passes. Counted in
cells rather than distinct pairs -- a settings screen is painted from the fixed
ScreenPalette and is legitimately down to four pairs (F1's composer), while the
thinnest real frame still paints 201 glyphs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
Member
Author
|
Taken, though not in the shape suggested — and the premise was right, so worth recording what the measurement said.
Measured before changing anything: on all 72 genuine per-view frames the linear walk and the grid walk agree on the pair set exactly. That is expected rather than lucky — every painted cell appears in the stream, so the stream is a superset of the screen. The error direction was therefore a false alarm on a colour nobody sees, never a missed offender, which is the safe way round for an audit. So this is a tidy-up that removes a caveat, not a fix for a live hole. One thing did come out of chasing it that is worth having: swapping walkers is exactly the change that can make this suite vacuous, since a walker handed the wrong dimensions decodes nothing and an audit over nothing passes every assertion under it. There is now a floor on painted cells. It is counted in cells rather than distinct pairs deliberately — a settings screen is painted from the fixed
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b1d3149-860c-462c-9a89-28c6e9b4f79a
📒 Files selected for processing (2)
tests/SharpMUTerm.Tui.Tests/FrameContrastTests.cstests/SharpMUTerm.Tui.Tests/FrameGrid.cs
CodeRabbit was right that ApplySgr mistracked three legal sequences: `CSI m`
(ECMA-48 makes it `CSI 0 m`, and splitting an empty string yields no codes, so
the loop ran no body and both colours stayed standing), and `39`/`49`, which
return one channel to default. None appears in any frame this driver emits --
19,681 SGR sequences across the 72 frames, all of them explicit `0;38;2;…` --
so it corrected no live reading. It is fixed because this walker is what every
suite asking about painted cells goes through.
Chasing it found a live one, and my first fix for it was wrong in the same way.
Backgrounds tested for the reset with `parameters.Contains("49")`, which reads
the 49 in a truecolor *argument* -- `38;2;49;5;6`, a foreground whose red channel
is 49 -- as the reset code and clears a background that sequence never mentions.
Splitting on `;` does not see that either: only a walk that consumes `38;2;r;g;b`
as one unit can tell an SGR code from a colour argument.
So Backgrounds is now a projection of the same walk Cells is, which is what this
file's own remarks already argued for ("three copies of a parser can drift into
disagreeing about which cells are painted, and the failure that produces is a
suite going quietly green on a frame it has misread"). It was three; it is one.
FrameGridCellsTests covers the walk directly on hand-written frames. Three of
its nine fail against the unfixed parser, verified by reverting it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/SharpMUTerm.Tui.Tests/FrameGrid.cs (1)
228-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat omitted SGR parameters as reset parameters.
Line 228 removes empty fields. Therefore,
CSI ; mproduces no SGR codes and preserves the active colours. Omitted SGR parameters default to zero, so this sequence must reset the colours.Map empty fields to
0instead of filtering them out. Add direct coverage for"\u001b[;m".Proposed fix
var codes = parameters.Split(';') - .Where(p => p.Length > 0) - .Select(p => int.Parse(p, CultureInfo.InvariantCulture)) + .Select(p => p.Length == 0 + ? 0 + : int.Parse(p, CultureInfo.InvariantCulture)) .ToList();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/SharpMUTerm.Tui.Tests/FrameGrid.cs` around lines 228 - 231, Update the SGR parameter parsing around the codes construction to retain empty fields and map each omitted field to integer 0 instead of filtering it out, so CSI ; m resets colours. Add direct test coverage for the escape sequence "\u001b[;m".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/SharpMUTerm.Tui.Tests/FrameGrid.cs`:
- Around line 228-231: Update the SGR parameter parsing around the codes
construction to retain empty fields and map each omitted field to integer 0
instead of filtering it out, so CSI ; m resets colours. Add direct test coverage
for the escape sequence "\u001b[;m".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 624b85e3-0727-4ced-8ae1-b76d249d5e16
📒 Files selected for processing (2)
tests/SharpMUTerm.Tui.Tests/FrameGrid.cstests/SharpMUTerm.Tui.Tests/FrameGridCellsTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two reported defects that share a cause.
The second is mostly the first: a highlight rule does fire, and the colour it fires in is invisible.
The measurement
Contrast (WCAG) of each colour against the plane it is actually painted on. The F2 highlight picker's palette:
Six of sixteen fail on dark, nine on light, and only
greyclears 3:1 on both. That is the finding the design turns on: a palette of fixed hexes cannot serve two themes, so the resolution has to happen where the colour meets its plane rather than where it is chosen.The reported bug is one cell of the same grid — the freeze bar took its accent from the theme's index 5 and painted it on the pane:
#800080on a#36363dfocused pane is 1.27:1. And the client's own hexes were all picked against a dark theme, so on Light the accent measured 1.42:1, the draft pen 1.26:1 and the notice 1.73:1. The Light theme's chrome has never been readable, and no snapshot showed it because every frame in the gallery renders Dark.The rule
Core.Text.Contrast— WCAG luminance and ratio, plusLegible(fg, plane, floor): the foreground moved the smallest distance that clears the floor, returned byte-identical when it already does.#808080looks like half way written down and is 0.216.#0000ffhas a relative luminance of 0.0722 and tops out at 1.88:1 on a dark pane at full blue.MarkupFormatterapplies it to every foreground it paints — against the span's own background when it has one, the theme's reading plane when it does not. That plane is per theme, not per pane: the extreme of the fourteen a pane can wear, which is the worst case rather than an approximation of one, so a focus change never re-formats a buffer. F7'skeep text legible(default on) switches it off to exactly the previous bytes.The client's own hexes become
ChromeInk, derived from the theme and held to the same floor.ScreenPaletteis deliberately untouched — those sit on the settings screens' own fixed backdrop, which no theme moves.What the frame audit found that reading the source did not
The design was implemented and then the paint was measured. Five more, each with a plausible-looking call site:
▌FrameContrastTestsis that audit as a test: every emitted SGR pair over 24 views × 3 themes. It exempts the powerline wedges and box-drawing rules (fill boundaries and dividers, not text), the solid blocks (F2's swatch is a colour sample, shown as the pane will paint it), and the framework's[dim]. The half blocks are not exempt —▌is the trigger rule and the focus marker, and one of them was a real defect this caught.One thing is outside the floor's reach and is named rather than hidden. SharpConsoleUI resolves
[dim]to a fixed#808080through no option we hold: 4.01:1 on Dark, 2.52:1 on Solarized Dark's focused pane. Reaching it means giving up[dim]across every renderer for an explicit floor-checked grey — a sweep, for a near miss on one theme. The exemption is a named predicate with the number in it.--theme <name>is new on the snapshot CLI, which is what makes the Light and Solarized frames verifiable at all.Triggers
Four changes. No schema change and no migration.
routegains an explicit(none)— the rule adds no destination and the line follows whatever the other matched rules decided. That is whatSpawnTarget = nullhas always meant; F2 labelled itmain, which reads as a destination, so "highlight it and leave it where it was" looked inexpressible. It is the default for a new rule.mainbecomes a real destination — the matching session's own window. It earns a reserved word rather than being spelt as the window's title because one trigger set is shared by every character that lists it and a title can only name one of them.main, soroute: main+ gag keeps the line where it used to delete it.WorldSessionraises oneSpawnLineper entry, so a highlight rule pointed at the same pane as its capture rule delivered every line twice.What is deliberately not changed: a highlight rule needs no route to reach the pane a capture rule sent the line to. There is one line and one set of destinations, and every matched rule's highlight is on it — which is the user's own framing ("it should still follow the original route, as long as it does not change where it routes to").
Verification
Frame-verified: the freeze bar's painted cells went from 1.27:1 to 3.02:1 on the default dark theme, still recognisably violet; and every one of the 72 frames the audit walks is clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
Summary by CodeRabbit
New Features
Bug Fixes